fix[lang]: forbid assignment to values#5181
Conversation
Gas ChangesNo changes detected. Summary
|
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22610a6845
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if not o.location: | ||
| raise StructureException("Looking for a variable location, instead got a value", expr) | ||
| # Looking for a variable location, instead got a value | ||
| assert o.location |
There was a problem hiding this comment.
This assert is still reachable when the lvalue is an address member rooted in a real variable, e.g. addr: address and self.addr.balance = 1 (or a local addr.balance = 1). The new semantic check only rejects DataLocation.UNSET, but get_expr_info for Attribute copies the STORAGE/MEMORY location from the address variable onto .balance, while parse_Attribute lowers .balance to a value IR node with no location. So these invalid assignments now surface as CodegenPanic/assertions instead of the intended user-facing StructureException; either keep the explicit exception here or reject address members during semantic analysis.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The correct fix for this would be to make <address>.balance (and other methods on address) have RUNTIME_CONSTANT mutability
But this is somewhat involved, as currently members cannot have a mutability different than what they are attached to
There was a problem hiding this comment.
Fixed by un-setting the location on "built-in members" of types
A more powerful and correct fix would be to have more granular control over the mutability of attribute accesses, this would allow us to potentially simplify the "not uses/initializes-ed" check (other_module.foo could be set as immutable), and to add private fields to modules (self.counter would be mutable in lib, but lib.counter would be immutable)
There was a problem hiding this comment.
(would also need a new Mutabillity, since addr.balance is neither CONSTANT nor RUNTIME_CONSTANT)
|
I'm not sure what the wording should be Ideas welcome ! |
|
lvalue, assignment target or even 'left hand side of an assignment' are all ok |
|
|
||
| if info.location == DataLocation.UNSET: | ||
| raise StructureException( | ||
| f"Cannot modify `{target.node_source_code}` since it is not a reference", target |
There was a problem hiding this comment.
| f"Cannot modify `{target.node_source_code}` since it is not a reference", target | |
| f"`{target.node_source_code}` is invalid as an assignment target", target |
2 participants
What I did
Fix #5138
Fix #5151
In short: we didn't check what was on the lhs of assignments enough, so things like
self.g()[0] = 5were allowed => failed assertsHow I did it
Check that the lhs of assignments has a location (memory/transient/storage), and if not, raise an exception.
This catches even tricky cases, like the long-standing
self.nostedArray.pop().append()which otherwise have correct type and mutabilityHow to verify it
pytestCommit message
Description for the changelog
Compiler no longer panics when trying to assign to expressions
Cute Animal Picture